refactor(server): tighten agent runtime + interactive SPI seams - #1091
Closed
FelixTJDietrich wants to merge 1 commit into
Closed
refactor(server): tighten agent runtime + interactive SPI seams#1091FelixTJDietrich wants to merge 1 commit into
FelixTJDietrich wants to merge 1 commit into
Conversation
Four architectural seams stretched but not torn by #1081. Tighten now while the context is fresh. - PiRuntimeFactory.build was a 160-line god method branching on runner kind: replace with a PiRunnerProfile strategy (PracticeRunnerProfile, MentorRunnerProfile) owned next to each adapter. The kernel reads three properties off the profile (runnerScript, nodeFlags, additionalEnv) instead of dispatching on filename. shouldRegisterHephaestusProvider stays a spec-helper — it depends on credentials/baseUrl, not runner kind. The profile interface is intentionally non-sealed: permits across packages would re-introduce the agent.runtime ↛ agent.practice/agent.mentor dependency the boundary rules forbid. Enforced architecturally via a new PiRunnerProfilePlacement rule, mirroring the ContentProvider pattern. Removed the unused operator-overridable runnerScript fields from PiAgentProperties / MentorAgentProperties — they decoupled the script filename from the V8 flags / per-process env it must pair with. - Per-thread frame routing was consumer-side filtering inside MentorRunnerClient. Push it into the SPI as AttachedSandbox.subscribe(Cursor, Predicate<JsonNode>, Consumer): the predicate runs server-side before queue enqueue, so suppressed frames consume neither queue capacity nor drop-counter budget. Broadcast frames (notifications without a thread destination) cross every filter — codified in MentorFrameFilters.forThread + SPI Javadoc. The two-arg overload remains as a default delegating to an always-true filter. - JsonlStdinWriter used synchronized under virtual-thread callers; replace with a ReentrantLock (mirrors MentorSseChannel#writeLock). JEP 444 pin risk removed without changing the enqueue-vs-terminal invariant. - PiPlanSpec.extraInputs accepted arbitrary workspace paths; validate at construction against WorkspaceAbi.allowedExtraInputPaths + allowedExtraInputPrefixes. Adapters caught writing to undeclared paths fail at boot/test time with a message naming the offending path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
Contributor
📚 Documentation Preview
|
FelixTJDietrich
added a commit
that referenced
this pull request
May 17, 2026
Items folded in from closed PRs #1090, #1091 — they belong here with the Pi runner config staging consolidation, not as standalone follow-ups. - WorkspaceAbi.MENTOR_SYSTEM_PROMPT_PATH constant; MentorPiAdapter writes through it (no more bare string literal scattered across Java + JS) - WorkspaceAbiSyncTest extended to scan pi-mentor-runner.mjs; envelope mismatch exits 42 (matching EXIT_ENVELOPE_MISMATCH) rather than the legacy hardcoded 2 - PiRunnerProfile (non-sealed; preserves runtime ↛ practice/mentor boundary) — PracticeRunnerProfile + MentorRunnerProfile own their own node flags, env, provider-registration. PiRuntimeFactory.build() delegates, no more switch-on-kind god method - TaskSealedHierarchyTest restored as architecture-tag guardrail against permit/JsonSubTypes drift; Task.PracticeReview gains its @JsonTypeName Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
FelixTJDietrich
added a commit
that referenced
this pull request
May 17, 2026
Items folded in from closed PRs #1089, #1090, #1091 — they belong here alongside the sticky-cookie + replica-affinity work, not as standalone follow-ups. - MentorReplicaAffinityCheck HealthIndicator (refuses to start on multi-replica without operator-asserted sticky routing) - WorkspaceContextBuilder mentor striping: hash (contributorId, workspaceId) instead of returning null and collapsing every mentor session onto stripe 0 - AttachedSandbox.subscribe(Cursor) — collapse the default-method subscribeFromNow seam; reused sandboxes were replaying prior turn's agent_end - Per-thread frame routing pushed into the SPI as Predicate<JsonNode> - JsonlStdinWriter synchronized → ReentrantLock (JEP 444 pin risk under virtual-thread callers) - FrameRingBuffer dropped_total metric with bounded cardinality - EvictionReason.MAX_LIFETIME wired (165 MB RSS × N idle users) - MentorChatMetrics.Outcome.CAPACITY_EXCEEDED so dashboards stop being blind to per-user-cap rejections Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on PR #1090 (
post-1081-correctness-sweep). Tightens four architectural seams stretched by #1078 / #1080 / #1081 while the context is fresh — every change is mechanical, behaviour-preserving, and gated by a test.1.
PiRuntimeFactory.buildis no longer a god methodThe 160-line
build()previously branched on runner kind via filename match (MENTOR_RUNNER_SCRIPTconstant +nodeFlagsFor/nodeEnvFor/shouldRegisterHephaestusProviderswitches). Adding a third runner kind meant editing four switches in the kernel.PiRunnerProfileinterface inagent.runtime; implementationsPracticeRunnerProfile(inagent.practice) andMentorRunnerProfile(inagent.mentor) own the runner-specific V8 flags + per-process env next to their adapters.shouldRegisterHephaestusProvider(spec)deliberately stays a kernel-side helper — it's a function of credentials/baseUrl, not runner kind, and both profiles compute it identically.sealed ... permits PracticeRunnerProfile, MentorRunnerProfilewould forceagent.runtimeto import its domain implementations — exactly the dependency the boundary rules forbid. Enforced architecturally instead via a newPiRunnerProfilePlacementrule, mirroring the establishedContentProviderpattern.runnerScriptfield fromPiAgentProperties/MentorAgentProperties— it decoupled the script filename from the V8 flags / per-process env it must pair with.2. Per-thread frame routing pushed into the SPI
MentorRunnerClientused to filter inbound frames byboundThreadIdinsidehandleEvent. A future logical-multiplex caller would have had to reinvent the same wheel.AttachedSandbox.subscribe(Cursor, Predicate<JsonNode>, Consumer)with server-side filtering: the predicate runs insideFrameSubscription.offerbefore queue enqueue, so rejected frames consume neither queue capacity nor drop-counter budget.frame -> true.MentorFrameFilters.forThread(threadId)encapsulates the JSON-RPC-aware filter — broadcast frames (notifications without aparams.threadId) cross every filter; only frames with a mismatched threadId are dropped.3.
JsonlStdinWriterno longer usessynchronizedunder virtual threadsMentor's
send()runs on virtual threads; the previoussynchronizedblocks risked JEP 444 pinning the carrier OS thread for the full critical section (incl. inner queue ops). Replaced with aReentrantLock— mirrors the precedent set byMentorSseChannel#writeLock. Behaviour identical; pin risk gone.4.
PiPlanSpec.extraInputspath-validation guardAdapters could previously write to arbitrary workspace paths. Now
extraInputskeys must be either:CONTEXT_TARGET_PREFIX,SESSIONS_DIR_PREFIX), orWorkspaceAbi.allowedExtraInputPaths()(currentlyMENTOR_SYSTEM_PROMPT_PATHfrom fix(server): post-#1081 correctness sweep #1090).Violations fail at construction time (boot / first unit test) with a message naming the offending path. Forces new mount points to be declared on
WorkspaceAbi.Test plan
./mvnw test -Dsurefire.includedGroups="unit,architecture"— 2683 tests (2556 unit + 127 architecture), all green.PiRuntimeFactoryTestparametrised on both profiles,MentorFrameFiltersTest,AttachedSandboxFilterTest,JsonlStdinWriterConcurrencyTest,PiPlanSpecValidationTest.git grep -n "synchronized" server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/JsonlStdinWriter.java→ 0git grep -n "MENTOR_RUNNER_SCRIPT" server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/runtime/→ 0AgentRuntimeBoundaryTestextended withPiRunnerProfilePlacementrule — passes.Stacking notes
Base branch is
post-1081-correctness-sweep(PR #1090). The Cursor enum +MENTOR_SYSTEM_PROMPT_PATHconstant + correctness fixes from that PR are dependencies, not duplicated here.🤖 Generated with Claude Code